-
Notifications
You must be signed in to change notification settings - Fork 141
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Refactor: Quote variables in preview of _forgit_diff, _forgit_reset_head & _forgit_cherry_pick_form_branch #372
Conversation
bin/git-forgit
Outdated
@@ -263,7 +263,7 @@ _forgit_diff() { | |||
opts=" | |||
$FORGIT_FZF_DEFAULT_OPTS | |||
+m -0 --bind=\"enter:execute($FORGIT diff_enter {} $escaped_commits | $_forgit_enter_pager)\" | |||
--preview=\"$FORGIT diff_view {} $_forgit_preview_context $escaped_commits\" | |||
--preview=\"$FORGIT diff_view {} \"$_forgit_preview_context\" $escaped_commits\" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really the correct thing to do here? As far as I can see we're assigning the opts
variable, so the escaped quotes will be a quote in the variable's value, which would make this line:
--preview="$FORGIT diff_view {} "$_forgit_preview_context" $escaped_commits"
So actually this does not really quote the variable in the --preview
argument. Or am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is. From my understanding, fzf
evaluates the string that is passed to --preview
, which essentially removes the outer quotes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But evaluating the string would also remove the inner quotes, because the first one of them closes the very first quote, and the second one opens a new quote to be closed by the very last quote. Wouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you point now and you're right. I think the correct way to escape this would be like this then:
--preview=\"$FORGIT diff_view {} \\\"$_forgit_preview_context\\\" $escaped_commits\"
Do you agree?
We would also have to update several other lines that make the same mistake, e.g. in _forgit_reset_head
we do the same thing
opts="
$FORGIT_FZF_DEFAULT_OPTS
-m -0
--preview=\"$FORGIT reset_head_preview \"$rootdir\"/{}\"
$FORGIT_RESET_HEAD_FZF_OPTS
"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad you're seeing it as well. You're totally right, we should fix it everywhere. The double escape you suggested seems correct. It's hardly readable, though. Would single quotes work here instead? Or would that be bad practice? It shouldn't prevent the shell variables from being evaluated because that happens when assigning opts
already, so the --preview
argument would have constant values in it. We could choose if we'd either replace the outer or the inner double quotes with single quotes. WDYT?
…ead & _forgit_cherry_pick_form_branch
Good idea to use single quotes, looks way nicer this way @carlfriedrich |
@sandr01d I just did a quick
It works anyway, but it's still not what we want, I guess. |
@carlfriedrich |
Looks good to me now, thanks for the update! |
Check list
Description
The
$_forgit_preview_context
variable used in the deferred code block for generating the preview of_forgit_diff
was not quoted. No known bugs were caused by this, just making sure we follow best practices.Note
This is a follow-up to #326 and was originally discussed here.
Type of change
Test environment